fix: harden browser tools start and console flag by ShiroKSH · Pull Request #28 · steipete/agent-scripts

ShiroKSH · 2026-07-09T18:09:33Z

Summary

replace shell-built start setup commands with argv-based process/filesystem calls
copy Chrome profile directories with Node filesystem APIs instead of rsync
fix console no-serialize handling so Commander negated option is honored

Why

browser-tools start accepted profile paths and interpolated them into shell command strings. A path containing shell metacharacters could change the invoked command. The console command also declared no-serialize as a negated Commander option but read a non-existent noSerialize property, so the flag did not affect serialization.

Validation

built patched binary with Bun
smoke-tested root, start, and console help on the compiled binary
live start profile smoke with a temporary profile source and a profile destination containing a shell metacharacter
live console capture comparison showing serialized object output by default and raw text output with no-serialize

Live Proof

Redacted terminal output from the patched compiled binary:

proofRoot=[temp]/browser-tools-proof-75a5d014743241028a8abd5f9d93678c
port=35736
✓ Chrome listening on http://localhost:35736 (profile copied)
profileLocalStateCopied=True
browser=Edg/149.0.4022.80
✓ Opened in new tab: data:text/html,<html><body>browser-tools proof</body></html>
serializedConsole:
Capturing console logs for 2 seconds...
[LOG] 21:19:17.992 { alpha: 1, nested: { beta: 2 } }
rawConsole:
Capturing console logs for 2 seconds...
[LOG] 21:19:20.239 [object Object]

clawsweeper · 2026-07-09T18:10:19Z

Codex review: needs maintainer review before merge. Reviewed July 11, 2026, 7:30 PM ET / 23:30 UTC.

Summary
The PR replaces shell-interpolated browser profile setup with direct process/filesystem APIs, adds profile-copy and launcher compatibility tests, and fixes Commander --no-serialize handling.

Reproducibility: yes. The maintainer reproduced command substitution through current main's shell-built --profile-dir path, and source inspection independently confirms both that interpolation and the incorrect Commander property read.

Review metrics: 2 noteworthy metrics.

Compatibility coverage: 6 tests, 16 expectations reported. The exact-head proof exercises the destructive filesystem cases that generic lint or security checks cannot settle.
Patch surface: 3 files; 179 additions, 9 deletions. The change remains focused on one browser-tools workflow despite substantial defensive logic and regression coverage.

Merge readiness
Overall: 🦞 diamond lobster
Proof: 🦞 diamond lobster
Patch quality: 🦞 diamond lobster
Result: ready for maintainer review.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Risk before merge

[P1] Merging deliberately changes how existing profile destinations are emptied and refreshed, including symlinked directories; the exact-head tests and live proof substantially reduce this compatibility risk but do not make the operation non-destructive.

Maintainer options:

Merge the exact proven head (recommended)
Accept the intentional profile replacement behavior because this exact candidate has focused compatibility tests and live proof across source, destination, symlink, overlap, console, and launcher paths.
Pause if the head changes
Require a fresh targeted review and repeat the filesystem/runtime proof if additional commits alter profile-copy or launcher semantics before merge.

Next step before merge

[P2] The active exact-head PR is already repaired and well proven; the remaining action is ordinary maintainer merge handling rather than a separate automated fix job.

Security
Cleared: The diff removes user-controlled shell interpretation and adds no dependencies, workflow permissions, secret access, downloads, package-resolution changes, or new third-party execution.

Review details

Best possible solution:

Merge the exact proven head while retaining the focused profile-copy tests as the compatibility contract for source validation, overlap rejection, symlink preservation, destination replacement, and launcher execution.

Do we have a high-confidence way to reproduce the issue?

Yes. The maintainer reproduced command substitution through current main's shell-built --profile-dir path, and source inspection independently confirms both that interpolation and the incorrect Commander property read.

Is this the best way to solve the issue?

Yes. Direct filesystem/process APIs remove shell interpretation at the root, and the follow-up compatibility guards and tests address the previously found symlink and destructive-copy regressions without adding a parallel implementation path.

AGENTS.md: not found in the target repository.

Codex review notes: model internal, reasoning high; reviewed against 5c6abe6de9a2.

Label changes

Label changes:

add rating: 🦞 diamond lobster: Overall readiness is 🦞 diamond lobster; proof is 🦞 diamond lobster and patch quality is 🦞 diamond lobster. Replaced prior rating: 🦐 gold shrimp.
remove rating: 🦐 gold shrimp: Current PR rating is rating: 🦞 diamond lobster, so this older rating label is no longer current.

Label justifications:

P2: The PR fixes a concrete command-injection path and broken CLI flag in a bounded optional browser-tools workflow.
merge-risk: 🚨 compatibility: The patch changes existing profile destination replacement, symlink handling, and launcher detection during startup.
rating: 🦞 diamond lobster: Overall readiness is 🦞 diamond lobster; proof is 🦞 diamond lobster and patch quality is 🦞 diamond lobster. Replaced prior rating: 🦐 gold shrimp.
status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action. Sufficient (terminal): The PR contains redacted after-fix terminal output, and the maintainer supplied exact-head live Chrome, filesystem, console, compiled-launcher, and regression-suite proof with an observed improved result.
proof: sufficient: Contributor real behavior proof is sufficient. The PR contains redacted after-fix terminal output, and the maintainer supplied exact-head live Chrome, filesystem, console, compiled-launcher, and regression-suite proof with an observed improved result.

Evidence reviewed

What I checked:

Shell interpretation removed: Current main interpolates profileDir into mkdir and rsync shell strings, while the candidate uses direct filesystem calls and passes Chrome arguments through spawn. (scripts/browser-tools.ts:119, e281904c32e2)
Previous finding resolved: The exact head preserves relative symlink text with verbatimSymlinks: true; its regression test asserts that a copied relative link still targets target. (scripts/browser-tools.ts:85, e281904c32e2)
Destructive boundary guarded: The candidate validates the source before destination mutation, rejects source/destination overlap, preserves directory symlink destinations, and safely replaces links to non-directories; focused tests cover each behavior. (scripts/browser-tools-profile.test.ts:25, e281904c32e2)
Exact-head live proof: The maintainer reports live Chrome 150 validation of literal shell metacharacters, profile copying, relative and top-level symlinks, linked destinations, overlap rejection, both console serialization modes, compiled launchers, and 6 tests with 16 expectations on this exact candidate. (e281904c32e2)
GitHub merge state: GitHub reports the exact head as mergeable against the stated base; the available GitGuardian security check passed, although the aggregate merge state is unstable. (e281904c32e2)
Feature provenance: Git history ties browser startup to Peter Steinberger's original browser-tools work and console serialization controls to the merged console-command contribution. (scripts/browser-tools.ts:110, 35355d0d3bae)

Likely related people:

steipete: Introduced the browser-tools startup behavior, recently maintained the file, authored four compatibility follow-ups on this candidate, and supplied exact-head live proof. (role: feature owner and recent area contributor; confidence: high; commits: 35355d0d3bae, 9ff786db80b4, 2edc27ba104b; files: scripts/browser-tools.ts, scripts/browser-tools-profile.test.ts)
srigi: The merged console-command work introduced the serialization option surface that this PR corrects. (role: console feature introducer; confidence: medium; commits: ea0e72062947; files: scripts/browser-tools.ts)

What the crustacean ranks mean

🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
🌊 off-meta tidepool: rating does not apply to this item.

Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

How this review workflow works

ClawSweeper keeps one durable marker-backed review comment per issue or PR.
Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
Maintainers can also comment @clawsweeper review to request a fresh review only.
Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

Review history (4 earlier review cycles)

reviewed 2026-07-09T18:13:18.377Z sha 8524425 :: needs real behavior proof before merge. :: none
reviewed 2026-07-09T18:23:30.368Z sha abe8545 :: needs maintainer review before merge. :: none
reviewed 2026-07-09T18:28:54.224Z sha abe8545 :: needs maintainer review before merge. :: none
reviewed 2026-07-10T19:49:21.685Z sha abe8545 :: needs changes before merge. :: [P1] Preserve relative symlinks when copying the Chrome profile

ShiroKSH · 2026-07-09T18:20:41Z

Proof added to the PR body.

@clawsweeper re-review

clawsweeper · 2026-07-09T18:20:44Z

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

steipete · 2026-07-11T23:26:48Z

Maintainer proof for exact candidate e281904c32e2641ce522582481e49bc2f83dbfef on base 5c6abe6de9a2dedd370dbc3e0e7779b56c17d2c6:

Current-main reproduction: a command substitution embedded in --profile-dir executed through the former shell-built setup path.
Exact candidate browser runtime and regression-test blobs are byte-identical to the fully live-proven candidate.
Live Chrome 150 proof on macOS: literal shell metacharacters remained path data with no marker side effect; profile copy and navigation succeeded; relative and top-level source symlinks were preserved; a symlinked destination remained linked and refreshed; overlapping paths were rejected without source loss.
Console proof: default capture serialized nested objects; --no-serialize emitted raw object text.
Browser/profile regression suite: 6 tests, 16 expectations, all passed.
Standalone compiled Bun binary and symlinked launcher help: passed. Node 22.12 and Node 20 direct/symlinked TypeScript launcher proof remains exact because the runtime blob is unchanged.
Full repository gates: 66-skill validation, docs listing, orchestrator policy test, ClawSweeper status tests, npm auth tests, skill-cleaner tests, and git diff --check all passed.
Dependency audit: 0 vulnerabilities across the resolved production graph.
AutoReview against current origin/main: clean, zero accepted/actionable findings, correctness confidence 0.96.
Public Model Identifier Gate: PASS. Candidate diff, tests, fixtures, compiled proof artifact, commit messages, existing public PR discussion, validation output, and this proof text contain no model identifiers requiring publication review.

Risk: medium. The change deliberately replaces browser-profile contents, but source validation, overlap rejection, symlink-preserving behavior, malformed-destination handling, and regression coverage now protect the destructive boundary.

steipete · 2026-07-11T23:27:54Z

@clawsweeper re-review

clawsweeper · 2026-07-11T23:27:57Z

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

Re-review progress:

State: Complete
Detail: The item is closed, terminal review leases were released, and no stale verdict was published or routed.
Run: https://github.com/openclaw/clawsweeper/actions/runs/29172259197
Updated: 2026-07-11T23:35:51.542Z

steipete

Maintainer review at exact head e281904c32e2641ce522582481e49bc2f83dbfef: shell-injection reproduction, profile-copy safety and compatibility regressions, console behavior, full tests, live Chrome proof, dependency audit, exact-head security check, ClawSweeper re-review, and model-identifier gate are clean. Approved for squash merge.

steipete · 2026-07-11T23:33:16Z

Landed as 5cc32f9. Thank you, @ShiroKSH.

Verification:

Current-main shell-command reproduction established the original profile-path injection boundary.
Browser/profile regression suite: 6 tests, 16 expectations, all passed.
Compiled Bun CLI, direct and symlinked launchers, full repository tests, and dependency audit passed.
Live Chrome 150 proof covered path safety, profile copy/navigation, symlink preservation, overlap rejection, and both console serialization modes.
AutoReview and exact-head ClawSweeper re-review: clean; no changes requested.
Exact-head GitGuardian check: passed.
Post-merge CI: https://github.com/steipete/agent-scripts/actions/runs/29172280605 — smoke passed.
Public Model Identifier Gate: PASS.

No remaining caveats.

ShiroKSH force-pushed the fix/browser-tools-start-console branch from 8524425 to abe8545 Compare July 9, 2026 18:19

ShiroKSH marked this pull request as ready for review July 10, 2026 19:28

ShiroKSH and others added 5 commits July 11, 2026 16:23

fix: harden browser tools start

609f219

fix: preserve browser profile symlinks

2edc27b

fix: guard browser profile copy targets

d4f6dbf

fix: preserve browser tools launcher compatibility

96bc492

fix: preserve profile destination semantics

e281904

steipete force-pushed the fix/browser-tools-start-console branch from abe8545 to e281904 Compare July 11, 2026 23:26

steipete approved these changes Jul 11, 2026

View reviewed changes

steipete merged commit 5cc32f9 into steipete:main Jul 11, 2026
1 check passed

Navigation Menu

fix: harden browser tools start and console flag - #28

fix: harden browser tools start and console flag#28
steipete merged 5 commits into
steipete:mainfrom
ShiroKSH:fix/browser-tools-start-console

ShiroKSH commented Jul 9, 2026 •

edited

Loading

Uh oh!

clawsweeper Bot commented Jul 9, 2026 •

edited

Loading

Uh oh!

ShiroKSH commented Jul 9, 2026

Uh oh!

clawsweeper Bot commented Jul 9, 2026 •

edited

Loading

Uh oh!

steipete commented Jul 11, 2026

Uh oh!

steipete commented Jul 11, 2026

Uh oh!

clawsweeper Bot commented Jul 11, 2026 •

edited

Loading

Uh oh!

steipete left a comment

Uh oh!

Uh oh!

steipete commented Jul 11, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

ShiroKSH commented Jul 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Why

Validation

Live Proof

Uh oh!

clawsweeper Bot commented Jul 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ShiroKSH commented Jul 9, 2026

Uh oh!

clawsweeper Bot commented Jul 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

steipete commented Jul 11, 2026

Uh oh!

steipete commented Jul 11, 2026

Uh oh!

clawsweeper Bot commented Jul 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

steipete left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

steipete commented Jul 11, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ShiroKSH commented Jul 9, 2026 •

edited

Loading

clawsweeper Bot commented Jul 9, 2026 •

edited

Loading

clawsweeper Bot commented Jul 9, 2026 •

edited

Loading

clawsweeper Bot commented Jul 11, 2026 •

edited

Loading